Prevent unresolvable SVG attributes from dropping the entire generated asset#1320
Prevent unresolvable SVG attributes from dropping the entire generated asset#1320jonathanmos wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the React Native SVG Babel plugin’s asset generation so that unsupported elements or non-statically-resolvable JSX attributes don’t cause the entire SVG to fail SVGO optimization (and therefore fail capture for Session Replay).
Changes:
- Update
RNSvgHandlerto treat unsupported SVG children as removable (signal + splice) rather than leaving them in the cloned AST. - Update
handleRegularAttributes()to (a) preserve statically-resolved falsy values like0, and (b) signal removal for unresolved JSX expression container attributes (e.g.fill={color}). - Add regression tests covering unsupported elements, dynamic/unresolvable attributes, and the
opacity={0}falsy-resolution case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react-native-babel-plugin/test/react-native-svg.test.ts | Adds/updates tests validating unsupported-element removal and non-resolvable attribute dropping during SVG capture. |
| packages/react-native-babel-plugin/src/libraries/react-native-svg/processing/attributes.ts | Makes regular attribute handling preserve falsy resolved values and remove unresolved JSX expression container attributes. |
| packages/react-native-babel-plugin/src/libraries/react-native-svg/handlers/RNSvgHandler.ts | Splices unsupported child elements out of the cloned SVG tree and removes unresolved expression-container attributes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
4f31770 to
de6f7b8
Compare
cdn34dd
left a comment
cdn34dd
left a comment
There was a problem hiding this comment.
Summary of on-call discussion for reference :
We should be careful when removing properties and tags that cannot be resolved from an SVG, as doing so can make the resulting SVG invalid and potentially break replays by affecting its bounds.
Consequently, this PR should shift its focus from handling both unresolvable properties and tags to simply handling tags, which offers a simpler and more focused improvement.
For tags, the goal is to drop any SVG tag that is not in our allowlist. This ensures that inner elements of AnimatedSVGs are removed while keeping all static components intact.
For dynamic properties that cannot be resolved at build time, we will address them in a separate PR. That follow-up will pass the runtime values of these properties through our custom native view to be filled in on the native side.
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
de6f7b8 to
055c694
Compare
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
a08cc87 to
643c43e
Compare
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
643c43e to
5c1669e
Compare
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
5c1669e to
be03aa6
Compare
jonathanmos
force-pushed
the
jmoskovich/fix-unresolvable-svg
branch
from
be03aa6 to
7f8fa11
Compare
What does this PR do?
An unsupported SVG tag inside an (e.g. Animated.createAnimatedComponent(Path), used by react-native-reanimated) was left in place with just a warning, so its untouched props got serialized into invalid SVG XML, svgo threw, and the entire silently disappeared from Session Replay instead of just the unsupported piece. This PR removes unsupported tags from the JSX tree before the SVG string is generated, so the rest of the tree is captured normally. Scope is intentionally limited to direct JSX children: properties that can't be resolved at build time (e.g. fill={color}), and unsupported tags reached only through a JSXExpressionContainer ({condition && }, {items.map(...)}), are both left untouched today and still cause the whole SVG to be skipped, same as before this PR — both are deferred to follow-up work rather than patched partially here.
Verified: the AnimatedPath checkmark icon is missing entirely before the fix, and renders as the circle with the checkmark absent after — with an unmodified control case (plain ) unchanged in both.
Before
After (notice I1 that has AnimatedPath)
Motivation
Previously, if any descendant of an had a dynamic attribute that couldn't be resolved at build time, or was a custom wrapper component not in the recognized SVG tag set, that raw JSX survived into the cloned AST. generate() would then serialize invalid SVG XML (e.g. fill={color}), svgo would throw on it, and the whole parent — not just the offending element — would silently fail to be captured for Session Replay. This reproduces a real customer pattern: an Animated.createAnimatedComponent-wrapped path with prop-driven fill/style.
The fix narrows the blast radius: only the specific unsupported element or unresolvable attribute is dropped, and the rest of the SVG generates normally.
Additional Notes
Added regression tests for: an unsupported element with a supported sibling, an unsupported element with dynamic/unresolvable attributes (the customer repro shape), and the falsy-zero (opacity={0}) case.
No behavior change for statically-resolvable attributes — those continue to be inlined as string literals as before.
Verified via tsc --noEmit, eslint, and the full react-native-babel-plugin test suite (190/190 passing).
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)